Share the OAuth refresh gate across execution stacks - #1537
Open
Rish-it wants to merge 5 commits into
Open
Conversation
The in-flight refresh gate lived inside a single scoped executor, but a self-host builds a fresh scoped executor per MCP session, so two sessions could each read the same stored refresh token and each believe it was the refresh winner. Providers that rotate refresh tokens reject the second redemption with invalid_grant and may revoke the whole token family, which kills the connection and forces reauthorization. The first refresh cycle still succeeds, so the fault stays hidden until a later expiry. Hang the gate off the shared root database handle so every scoped executor over one database converges on the same map, and include the tenant in its key: once the map spans tenants, owner/subject/integration/name alone would let two tenants collide on one entry. Dedup reaches one database handle in one process. A host that hands out a fresh handle per request, and any multi-replica deployment, still needs database-backed coordination; the boundary is documented at the gate.
Sharing the gate across execution stacks also shared the first caller's cancellation. The grant was memoized with Effect.cached, so the fiber that happened to register it ran it; interrupting that fiber completed the deferred with an interrupt and every peer awaiting the same entry failed with a cancellation none of them caused and none could act on. A disconnected MCP client, an execution deadline or a cancelled tool call was enough to take down an unrelated session mid-refresh. Run the grant with Effect.runFork and hand callers Fiber.join instead. Joining is per-caller, so a cancelled peer detaches without touching the grant or its siblings, and a grant nobody is left waiting on still settles and still persists the rotated token. Token requests are already bounded by AbortSignal.timeout, so the detached fiber cannot outlive its request. This also removes the check-and-set re-check: runFork and the map operations are synchronous, so there is no yield between the lookup and the registration and the sequence is already atomic against peer fibers.
Two MCP sessions against one self-host connection, both rejected by the upstream at the same instant: the upstream holds each session's first call until both have arrived, so the refresh contention is forced rather than left to the scheduler. The authorization server's own request ledger is the evidence — exactly one refresh grant, and both retries carrying the same new bearer.
Eight sessions through the same barrier upstream, then a second wave. The first wave shows the grant count does not scale with the session count; the second shows the gate is released once a grant settles rather than latched, since a latched gate would replay a retired token and a gate that never released would deadlock every later refresh. Neither failure mode is visible to a single-wave, two-session test.
There was a problem hiding this comment.
Pull request overview
This PR fixes cross-session OAuth refresh-token races in self-hosted deployments by sharing the in-flight refresh dedup gate across all scoped executors that use the same root DB handle, and by detaching the shared refresh grant from any single caller’s cancellation so one interrupted session can’t cancel peers.
Changes:
- Move the refresh in-flight map to a module-level
WeakMapkeyed by the root DB handle, and includetenantin the dedup key to avoid cross-tenant collisions. - Run the refresh grant on its own fiber and have all callers
join, preventing one caller’s interruption from failing all waiters. - Add unit + selfhost e2e scenarios to prove cross-session dedup and correct gate release across multiple “waves”.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/core/sdk/src/executor.ts | Shares refresh dedup state across executor stacks via root DB handle; switches to fork+join semantics and adds tenant to the gate key. |
| packages/core/sdk/src/oauth-flow.test.ts | Adds unit coverage for cross-executor-stack refresh dedup and interruption survivability. |
| e2e/selfhost/oauth-refresh-cross-session.test.ts | New selfhost e2e proving two MCP sessions join a single refresh grant. |
| e2e/selfhost/oauth-refresh-session-stress.test.ts | New stress e2e proving the gate releases after settle and holds across waves. |
| .changeset/oauth-refresh-cross-session.md | Patch changeset documenting the behavioral fix and its scope boundary. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+2073
to
+2082
| const running = Effect.runFork( | ||
| performTokenRefresh(row, provider, trigger).pipe( | ||
| Effect.ensuring(Effect.sync(() => refreshInFlight.delete(key))), | ||
| ), | ||
| ); | ||
| // Re-check after building (a peer fiber may have registered first while | ||
| // we built ours) so everyone converges on the same shared grant. | ||
| const winner = refreshInFlight.get(key) ?? gated; | ||
| if (winner === gated) refreshInFlight.set(key, gated); | ||
| return yield* winner; | ||
| // No `yield*` between the lookup above and this registration, so | ||
| // check-and-set is atomic against peer fibers and cannot double-fire. | ||
| const shared = Fiber.join(running); | ||
| refreshInFlight.set(key, shared); | ||
| return yield* shared; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Four self-hosted Todoist connections died with
invalid_grant: refresh token reuse detected; tokens for this client/user/resource revoked, each after a refresh cycle that had already succeeded once. Reported in #1520.The in-flight refresh gate from #367 was created inside
createExecutor, so it only ever covered one scoped executor. A self-host builds a fresh scoped executor per MCP session (makeScopedExecutor→mcp-build.ts), so two sessions resolving the same connection each read the same stored refresh token and each believed it was the refresh winner. Against a provider that rotates refresh tokens, the loser redeems a retired token and the AS revokes the family. The first refresh always succeeds, which is why the fault looks like a working integration until a later expiry.The gate now hangs off the shared root DB handle rather than the executor instance, so every scoped executor over one database converges on the same map. Its key gains the tenant: once the map spans tenants,
owner:subject:integration:namealone would let two tenants collide on one entry.Sharing the gate also shared the first caller's cancellation, so that is fixed in the same branch. The grant was memoized with
Effect.cached, which runs it on whichever fiber registered it — interrupting that fiber completed the deferred with an interrupt and failed every peer awaiting the same entry with a cancellation none of them caused. A disconnected MCP client or an execution deadline was enough to take down an unrelated session mid-refresh. The grant now runs viaEffect.runForkand callers getFiber.join, so a cancelled peer detaches without touching the grant or its siblings, and a grant nobody is left waiting on still settles and still persists the rotated token. Token requests are already bounded byAbortSignal.timeout, so the detached fiber cannot outlive its request. This also drops the check-and-set re-check —runForkand the map operations are synchronous, so there is no yield between lookup and registration.Evidence
Both e2e scenarios drive real MCP sessions against a booted self-host and assert on the authorization server's own request ledger, with the upstream holding each session's first call until all have arrived so the contention is forced rather than left to the scheduler.
oauth-refresh-cross-session: two sessions. Before, 2 refresh grants; after, 1, with both retries carrying the same new bearer.oauth-refresh-session-stress: eight sessions over two waves. Before, 8 grants; after, 2. The second wave is what proves the gate is released once a grant settles rather than latched — a latched gate replays a retired token, and one that never releases deadlocks every later refresh.oauth-flow.test.tsreproduces the reported failure directly: without the fix the second stack failsinvalid_grantwithreauthRequired: true. A second case covers the interruption path.Local runtimes were never affected — the CLI and desktop share one boot-built executor (
apps/local/src/executor.ts), so there is one map per process already.e2e/local/oauth-token-durabilitystill passes.Scope
Dedup reaches one root DB handle in one process, which is what #1520 asks for. It does not cover a host that hands every scoped executor a fresh handle — Cloud builds its FumaDB handle inside a per-request layer, and its MCP sessions are per-session Durable Objects — nor multi-replica self-host. Both need database-backed coordination or compare-and-swap on the stored token, which is the remaining bullet from the issue and is out of scope here. The boundary is documented at the gate rather than left implicit, since an unshared gate still behaves correctly for the one caller holding it and would otherwise fail silently.
Fixes : #1520